Do not filter out properties on inactive pages that also appear on active pages #155

annekainicUSDS · 2018-07-12T04:58:14Z

Description

Resolves this ticket #140.

There are functions that transform the form data before it can be submitted. This is done to remove view: fields and inactive fields. Inactive fields were considered to be any field on a page that was inactive. This resulted in a problem where fields that appeared on both inactive and active pages were being removed from the form submission data entirely.

This PR adds logic to check if a property on an inactive page is also on an active page before determining whether it should be removed from the submission data.

I tested this against vets-website to confirm the fix works. I'm also going to be adding unit tests.

dmethvin-gov

I think I need to learn more about the process of submitting the form to be a useful reviewer here but I'll figure it out eventually.

dmethvin-gov · 2018-07-12T12:54:37Z

src/js/helpers.js

@@ -21,6 +24,16 @@ export function getActivePages(pages, data) {
  return pages.filter(page => isActivePage(page, data));
 }

+export function getActiveProperties(activePages) {
+  let allProperties = [];


How about this?

export function getActiveProperties(activePages) { return new Set([].concat(...activePages.map(p => Object.keys(p.schema.properties))); }

Then below, you can check activeProperties.has(prop). If that's too obtuse I would at least push all the properties from all the pages onto the allProperties array and use _.uniq on them just once before returning. The current code will create and immediately discard one array and one object per page in the iteration.

I tried implementing this the way you suggested, which I liked! However I was running into an issue with unit tests failing. If the test data isn't complete and doesn't include schema for every page, we get the following error: Cannot read property 'properties' of undefined. That's why I included a check for if (page.schema) exists before trying to return the keys.

I'm not sure what the best workaround for this is. I tried adding a check for if (page.schema) again in this .map function, but it doesn't work. I could fix it by filling out complete test data for every unit test, but that seems like a significant burden. It also may have future issues; should the code fail completely if someone forgets to write a complete config and there's a page without schema? Or if someone is just setting up the initial scaffolding? Not sure the best way to handle this situation.

This is similar to the problem I had in #153 where the unit tests were too skeletal and not set up like a real form. I'd prefer to have a check up front that every page contains a schema so we don't need to include guards everywhere in the code for a case that definitely seems broken. Even if we just throw an error on the console it gives the developer an idea of what they did wrong, since we should be able to give a specific "Chapter X page Y property Z does not contain a required schema" message.

At some point since we're pulling in a JSON Schema validator we should be able to run formConfig through that validator if we are brave enough to write a schema for it. 💫

The only case I can think of where we might not have a schema is if we end up doing #150 and use a form page for the intro page. Even for that, if we need to we could eliminate the special case by injecting empty schema/properties into it.

I like the idea of checking once up front for the presence of schema. Where do you think that check should happen? I might create a separate ticket for that.

Good question. Is it enough to just check the formConfig and its related parts once at startup? If so maybe it could go at the top of FormApp. If we need/want to also check dynamic changes to formConfig and what's inside it, the first question I'd have is whether we are doing that directly or going through some sort of update function. If the latter then we could just validate the part that changed.

Oh, I meant to say that it sounds like a good idea to make a ticket.

dmethvin-gov · 2018-07-12T13:02:39Z

src/js/helpers.js

    return Object.keys(page.schema.properties)
      .reduce((currentData, prop) => {
-        return _.unset(prop, currentData);
+        if (!activeProperties.includes(prop)) {
+          _.unset(prop, currentData);


Isn't this just delete currentData.prop? There are additional features in _.unset like being able to unset an expression like _.unset("kinda.deep.thing[5]", currentData) but I don't think we want that here do we? Not sure if schema props are allowed to have chars like . and [ ] in them.

👍 I changed this, the only thing that hung me up for a bit is the syntax is delete currentData[prop]. Without the brackets it looks for a key literally called "prop".

dmethvin-gov · 2018-07-12T13:19:48Z

src/js/helpers.js

@@ -212,11 +225,16 @@ export function filterViewFields(data) {
  }, {});
 }

-export function filterInactivePages(pages, form) {
-  return pages.reduce((formData, page) => {
+export function filterInactivePages(inactivePages, activePages, form) {


The filterXxxPages led me to believe this function was doing something to pages but it's really removing data properties from the form.data object that contains data from all pages, right? Maybe filterInactivePageData?

Yep, that makes sense!

dmethvin-gov · 2018-07-12T13:26:35Z

src/js/helpers.js

+export function filterInactivePages(inactivePages, activePages, form) {
+  const activeProperties = getActiveProperties(activePages);
+
+  return inactivePages.reduce((formData, page) => {


This seems reversed, we're really reducing form.data aren't we? I know inactivePages happens to be iterable since it's an array but that confused me. (It's totally possible I have no idea how us-forms-system data processing works yet so I can be easily confused. 😺 )

If I had to describe what is supposed to happen here, I think it would be: "Remove all the props out of form.data that appear only in inactive pages." Is that right? Is the form data all flattened into a single-level object at this point?

dmethvin-gov

As I've been building out the "kitchen sink" version of a demo that covers more cases, it seems like we definitely can have nested schema inside schema on a page. The main concern I have is that we're only fixing this for the case of top-level stuff and not for nested stuff, but I have no idea if that's a valid concern or not. If it's not happening at the VA at least it's not an issue with our current one-and-only client. 😄 If you're okay with it I am too, we can always fix it if someone else reports it.

dmethvin-gov · 2018-07-13T18:40:08Z

src/js/helpers.js

@@ -21,6 +24,16 @@ export function getActivePages(pages, data) {
  return pages.filter(page => isActivePage(page, data));
 }

+export function getActiveProperties(activePages) {
+  let allProperties = [];


This is similar to the problem I had in #153 where the unit tests were too skeletal and not set up like a real form. I'd prefer to have a check up front that every page contains a schema so we don't need to include guards everywhere in the code for a case that definitely seems broken. Even if we just throw an error on the console it gives the developer an idea of what they did wrong, since we should be able to give a specific "Chapter X page Y property Z does not contain a required schema" message.

At some point since we're pulling in a JSON Schema validator we should be able to run formConfig through that validator if we are brave enough to write a schema for it. 💫

The only case I can think of where we might not have a schema is if we end up doing #150 and use a form page for the intro page. Even for that, if we need to we could eliminate the special case by injecting empty schema/properties into it.

dmethvin-gov · 2018-07-13T18:41:45Z

src/js/helpers.js

-        return _.unset(prop, currentData);
+        newData = currentData;
+        if (!activeProperties.includes(prop)) {
+          delete newData[prop];


Good catch on this, my brain wasn't working!

dmethvin-gov · 2018-07-13T18:52:18Z

src/js/helpers.js

+  let allProperties = [];
+  activePages.map(page => {
+    if (page.schema) {
+      allProperties = _.uniq(_.concat(Object.keys(page.schema.properties), allProperties));


This will still call _.uniq once for each page, which could be a lot of times. we can reduce it to just once by pushing all the properties into an array and de-duping once. It's a little easier to read than using a Set although it's probably slower, but that probably doesn't matter since it's only called once and even on a large form this shouldn't be horrible.

let allProperties = []; activePages.forEach(page => allProperties.push(...Object.keys(page.schema.properties)); return _.uniq(allProperties);

dmethvin-gov

LGTM*2!

… also appear on active pages Refactor Refactor Remove duplicate properties from array Return properties

annekainicUSDS · 2018-08-09T14:12:05Z

This PR was approved by the Vets.gov team (cc'ing @markgreenburg). Will merge.

…tive pages (#155) * Add functionality to not filter out properties on inactive pages that also appear on active pages Refactor Refactor Remove duplicate properties from array Return properties * Write unit tests for new functionality * Refactor from code review * Change _.uniq to only be called once * Run build on latest changes

@annekainicUSDS

* Relative links will be the end of me * Relative links to definitions directory * Two more * Add links to docs and starter app, tighten up other copy * Significantly faster cc @annekainicUSDS * Add a consoleSubmit option for debugging output (#153) * Add a consoleSubmit option for debugging output Fixes #135 * Use pre-push hook to ensure build artifacts are up to date (#151) Fixes #97 * Add docs for creating a simple form (#149) * Add docs for creating a simple form * Fix link (#157) * [WIP] Add placeholder topic for available components (#154) * Add placeholder topic for available components * Additional changes after discussion with @dmethvin-gov and @fatimanoorva * Add a consoleSubmit option for debugging output (#153) * Add a consoleSubmit option for debugging output Fixes #135 * Use pre-push hook to ensure build artifacts are up to date (#151) Fixes #97 * Add docs for creating a simple form (#149) * Add docs for creating a simple form * Add some code examples and explanations on components * Better headings * Revert "Better headings" This reverts commit 66f40ae. * Apply Dave's changes again because Git is hard * Big reorg * Add placeholder topic for available components * fixes few relative links * add "contributing to this project" section include: links to code of conduct, CONTRIBUTING.md, and instructions on how to join the forms listserv * Update shortdesc * More line breaks 😩 * A bit more elaboration courtesy of Anne * Field component props and what they do * Clarify the purpose of this description once and for all * Clarify this weird point * What are "they"? * Links back to README, fix markdown * Better headings * Add these links * Add a hopefully better description of the keepInPageOnReview property * Keep consistent with "review page" * Use Anne's language, needs clarification on the second sentence * Introduction component * Form footer * Prgoress bar * Title and subtitle * Update mini-toc with new combined section header * For example * Date fields * Hidden contextual information - needs info on how to specify cc @dmethvin-gov * Radio Button and Checkbox Group - needs a code sample cc @dmethvin-gov * Password usage guidelines - what do we want to say about this? * Just kidding, the previous commit was for required fields, this one's for password * Duplicate field validation * Conditional form fields * Small reword * Add widget descriptions (#166) * Complete table with widget information * Add more description for some components * Add usage info for error messages * Edits from review * Remove password description * Make review edits * Add remaining descriptions (#174) * Add remaining descriptions * Make updates from review * Update language * Refactor from review comments * Remove one word * Rename components topic * Remove hidden contextual information cc #173 * Minor cleanup * Set the property to `true` * Create the new individual files * Move content, turn table rows into sections for linking * Move content * Add more descriptions for widgets * Rename to "Available" rather than "Common" * Edits from @dmethvin-gov * Fix link to docs (#181) * Widget links as appropriate * Add guidance on using widgets * currencyUI * This is guidance for adding widgets, the previous commit was for definitions * Combine info more coherently * Remove less specific information * Definitions for widgets and... definitions * More verbose explanation of the example * Remove AddressField per @annekainicUSDS * Improve content on the form config (#186) * Separate out advanced information on how the form config works, and create new content at a beginner level for basic information you need to know about creating a form config * Move to new customization map * Update links after move * Rename new conceptual topic * Rename map * Update main README after renames and moves * Rename again * Copy edits * Update shortdesc cc @annekainicUSDS * Remove these sections * Not "our" * Small fixes * Fix misspelling * H1 header * Fake breadcrumbs * relative link * 😩 * Once more * Two dots * Map topic readme links * Relative links for every breadcrumb * Topic links * Add folder name * This is broken * Fix links * All these commits are going to have the same commit message * Add folder * Once more * All breadcrumbs * Small link fixes here * Fix relative link? * Remove directory * Add markdown extension * Relative link * One more level * All small fixes for "Building a form" * Small updates for "Customizing the library" * Add react * Lots of hardcoded links * Remove containing directory * Line link doesn't work * Not plural * Markdown extension * MARKDOWN EXTENSION * Fix the example for conditionally hiding fields (#183) This is simplified from the big form I'm building. * More detail on use of definitions (#188) * [WIP] More detail on use of definitions * Fix imports; further updates * Add autosuggest section * Add missing expandable group styles (#192) * Add basic tutorial (#194) * Add basic tutorial * Fix language * Add Dave's suggestions * Add Sheri's suggestions * More edits from Sheri * Edit images * Small edits * No apostrophe (#197) * Don't show back button on first page (#195) Routes in `pageList` mocks now reflect the way they really look, using a leading slash. Also cleans up FormPage/ReviewPage unit tests which had remnants of vets.gov user login. Fixes #177 * TOC at the top of the tutorial * Minor cleanup * Add readme links * Minor cleanup of review section * Add breadcrumbs * Add content to question issue template * Clarify a couple of questions * Typos * Do not filter out properties on inactive pages that also appear on active pages (#155) * Add functionality to not filter out properties on inactive pages that also appear on active pages Refactor Refactor Remove duplicate properties from array Return properties * Write unit tests for new functionality * Refactor from code review * Change _.uniq to only be called once * Run build on latest changes * Clean up and dry out the FormPage unit tests (#202) * Update node-sass to remove the node-gyp vuln (#213) * 1.1.0 * Fix merge conflict

dmethvin-gov reviewed Jul 12, 2018

View reviewed changes

dmethvin-gov approved these changes Jul 13, 2018

View reviewed changes

annekainicUSDS changed the title ~~WIP Do not filter out properties on inactive pages that also appear on active pages~~ Do not filter out properties on inactive pages that also appear on active pages Jul 13, 2018

annekainicUSDS mentioned this pull request Jul 16, 2018

transformForSubmit strips non-empty showPagePerItem pages #140

Closed

dmethvin-gov approved these changes Jul 16, 2018

View reviewed changes

annekainicUSDS force-pushed the do-not-filter-properties-from-active-pages branch from 086676b to 02f8bb0 Compare August 1, 2018 23:01

Kainic and others added 5 commits August 8, 2018 17:49

Add functionality to not filter out properties on inactive pages that…

c118392

… also appear on active pages Refactor Refactor Remove duplicate properties from array Return properties

Write unit tests for new functionality

0bd39d3

Refactor from code review

3cbc36c

Change _.uniq to only be called once

ae7aef1

Run build on latest changes

28fe309

annekainicUSDS force-pushed the do-not-filter-properties-from-active-pages branch from 02f8bb0 to 28fe309 Compare August 8, 2018 21:50

annekainicUSDS merged commit 8527e2c into master Aug 9, 2018

annekainicUSDS deleted the do-not-filter-properties-from-active-pages branch August 9, 2018 14:12

annekainicUSDS mentioned this pull request Aug 10, 2018

Release 1.1.0 #216

Merged

bernars-usa mentioned this pull request Aug 14, 2018

Create template for release notes #223

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not filter out properties on inactive pages that also appear on active pages #155

Do not filter out properties on inactive pages that also appear on active pages #155

annekainicUSDS commented Jul 12, 2018

dmethvin-gov left a comment

dmethvin-gov Jul 12, 2018

annekainicUSDS Jul 13, 2018

dmethvin-gov Jul 13, 2018

annekainicUSDS Jul 13, 2018

dmethvin-gov Jul 13, 2018

dmethvin-gov Jul 13, 2018

dmethvin-gov Jul 12, 2018

annekainicUSDS Jul 13, 2018

dmethvin-gov Jul 12, 2018

annekainicUSDS Jul 12, 2018

dmethvin-gov Jul 12, 2018

dmethvin-gov left a comment

dmethvin-gov Jul 13, 2018

dmethvin-gov Jul 13, 2018

dmethvin-gov Jul 13, 2018

dmethvin-gov left a comment

annekainicUSDS commented Aug 9, 2018

Do not filter out properties on inactive pages that also appear on active pages #155

Do not filter out properties on inactive pages that also appear on active pages #155

Conversation

annekainicUSDS commented Jul 12, 2018

Description

dmethvin-gov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmethvin-gov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmethvin-gov left a comment

Choose a reason for hiding this comment

annekainicUSDS commented Aug 9, 2018